-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Color variables #114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Color variables #114
Conversation
Kills text shadow and slightly darkens green
Darkens up the disabled state just a little to eliminate the need for a text-shadow
Darkens the green color used throughout to provide a bit more contrast and reduce fluorescence.
With the darker color, we need to make sure the blue outline isn't vibrating too much.
Focused objects should stay on top of everything in the z-order stack.
Conflicts: css/.primer-stats.md css/primer.css
Basing the default alert on the brand-blue variable
Alert error is now based on $brand-red.
Make alert-warn based on brand-orange
scss/_mixins.scss
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Color white should be written in hexadecimal form as #ffffff
|
All flash colours already pass AA which is a good thing. However, I think we need to take a step back and ask the following:
I find text on a coloured background hard to read, how about removing the background all together? 😄 |
I think it's important that the flash messages stand out from the rest of the page (e.g. not a white background) and are also aesthetically "pleasing" for lack of a better word. We can definitely change things around, but I think the current flash messages with rounded corners and subtle background colors still look better than just thick borders around a message. |
|
I agree with @bleikamp here, but I'm not sure the implementation in this PR is better than what we've got outside of the fact that it's built on brand-color variables. |
|
@bleikamp @aaronshekey Agreed. |
|
Yeah, basing the blue alert on our blue color variable feels great, but the other two feel off. |
|
Ok @mdo. Kept things based on variables, but swapped for the original colors you specified. There should be no visual changes with this pass. Instead of unique colors for borders, backgrounds, etc, they're defined using Sass color functions. IF we wanted to, we could swap all our color values based on a user-preference for high contrast modes. |
|
👍 |
|
@aaronshekey this may be a dumb question, but how did you reverse engineer all those colors into functions? Seems like it would be really difficult! |
|
@djch I eyeballed it using Arc90's tool http://sassme.arc90.com |
Settings cleanup update

This pull request is an attempt to base all button and alert colors on global color variables. In theory, should you want to change these high-level variables, you'd only have to do it one spot. This should lay some baby-step groundwork for changing some of the main colors to support better accessibility through higher contrast modes or perhaps, even theming.